feat(frontend): adds vip qr code modal #3991
Conversation
… into feature(frontend)/adds-vip-qr-code-modal
…r-codes-option-in-menu' into feature(frontend)/displays-vip-qr-codes-option-in-menu # Conflicts: # src/declarations/backend/backend.did.d.ts
…r-codes-option-in-menu' into feature(frontend)/displays-vip-qr-codes-option-in-menu
| <span class="w-full"><SkeletonText /></span> | ||
| {/if} | ||
|
|
||
| <ButtonGroup styleClass="flex-col sm:flex-row" slot="toolbar"> |
There was a problem hiding this comment.
not really sure about this: we don't do it in any of the modals, nor in any usage of the Buttons... if we are going to change it, better to do it in another PR for all the ButtonGroup components, for consistency
There was a problem hiding this comment.
@AntonioVentilii What exactly do you mean with
for all the ButtonGroup components, for consistency
I mean like this we just enable to add some custom styling to the element
There was a problem hiding this comment.
indeed, but why only for this modal? why not for all?
There was a problem hiding this comment.
It's defined like this in the design. The buttons should swap the alignment if the page gets smaller. To achieve this on this modal we need something like this.
Other modals do not need to swap the alignment
There was a problem hiding this comment.
still not convinced about this... may you please double-check with design (especially if it's ONLY for this modal)? furthermore, will the "Close" button go up or down when it is vertical (i.e. flex-col-reverse)?
There was a problem hiding this comment.
@AntonioVentilii @artkorotkikh-dfinity just wrote me that he updated the designs.
| /> | ||
| </div> | ||
|
|
||
| <span class="mb-4 block w-full pt-3 text-center text-sm text-tertiary"> |
There was a problem hiding this comment.
i think block is default, no?
There was a problem hiding this comment.
No, it's not default
src/frontend/src/tests/lib/components/qr/VipQrCodeModal.spec.ts
Outdated
Show resolved
Hide resolved
src/frontend/src/tests/lib/components/qr/VipQrCodeModal.spec.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| onMount(() => { | ||
| countdown = setInterval(intervalFunction, 1000); |
There was a problem hiding this comment.
shouldn't countdown be cleared on destroy?
There was a problem hiding this comment.
Thats a good point, thx
| countdown = setInterval(intervalFunction, 1000); | ||
| }); | ||
|
|
||
| generateCode(); |
There was a problem hiding this comment.
Why does this finds place at the root of the module? Shouldn't it start when the component is mounted?
There was a problem hiding this comment.
hmm yes this should be moved into the onMount function
Good input. I will change it that way that it only generates new codes as long as the tab is active. |
…s-vip-qr-code-modal # Conflicts: # src/frontend/src/lib/stores/modal.store.ts
| return; | ||
| } | ||
|
|
||
| if (retries !== maxRetries) { |
There was a problem hiding this comment.
To avoid an edge case issue, < maybe? Otherwise it might retries if the number of retries is bigger than the max retries.
| let counter = VIP_CODE_REGENERATE_INTERVAL; | ||
| let countdown: NodeJS.Timeout | undefined; | ||
| const maxRetries = 3; | ||
| let retries = 0; |
There was a problem hiding this comment.
For better readability, could you either add a comment indicating that this is for retries in case of errors or rename the variable to make it more explicit?
|
|
||
| const regenerateCode = async () => { | ||
| clearInterval(countdown); | ||
| await generateCode(); |
There was a problem hiding this comment.
shouldn't the retries be asserted here? With the current implementation, we might indeed prevent any further calls, but the countdown would still proceed. Not a strong opinion, just a thought.
There was a problem hiding this comment.
LGTM, thanks
Given that I stricly only reviewed the script tag of VipQrCodeModal.svelte, please also wait for @AntonioVentilii approval to merge this PR.
| <div class="mx-auto mb-4 aspect-square h-80 max-h-[44vh] max-w-full p-4"> | ||
| {#if nonNullish(code)} | ||
| <QRCode value={qrCodeUrl}> | ||
| <div slot="logo" class="flex items-center justify-center rounded-lg bg-white p-2"> |
There was a problem hiding this comment.
shouldn't slot="logo" go in the parent div two levels above?
<div slot="logo" class="mx-auto mb-4 aspect-square h-80 max-h-[44vh] max-w-full p-4">
{#if nonNullish(code)}
<QRCode value={qrCodeUrl}>
<div class="flex items-center justify-center rounded-lg bg-white p-2">There was a problem hiding this comment.
@AntonioVentilii No it is a part of the QRCode
| <span class="w-full"><SkeletonText /></span> | ||
| {/if} | ||
|
|
||
| <ButtonGroup styleClass="flex-col sm:flex-row" slot="toolbar"> |
There was a problem hiding this comment.
still not convinced about this... may you please double-check with design (especially if it's ONLY for this modal)? furthermore, will the "Close" button go up or down when it is vertical (i.e. flex-col-reverse)?
Motivation
Vip users should be able to create reward codes and get qr codes to show them to other clients.
Changes
Tests
qr code modal:
